Expose keyring trace in results #224
Conversation
|
|
||
|
|
||
| @attr.s | ||
| class CryptoResult(object): |
There was a problem hiding this comment.
Any value in calling this AwsCryptoResult to match Java?
There was a problem hiding this comment.
IMO no, but I'm willing to be convinced otherwise.
My reasoning is that aside from the name/branding and the AWS KMS keyring, nothing about the ESDK is AWS-specific. As such, I've tried to avoid including "AWS" in the names of things except where they specifically relate to AWS (ex: keyrings.aws_kms).
There was a problem hiding this comment.
Yeah, I'm fine with keeping it. As you know we changed the name more out of necessity than because it was a better name.
based on responses/ any other commits.| @@ -2,6 +2,24 @@ | |||
| Changelog | |||
| ********* | |||
|
|
|||
| 1.5.0 -- 2020-xx-xx | |||
There was a problem hiding this comment.
I assume this will be updated once it's shipped.
There was a problem hiding this comment.
Yup. That's out standard placeholder for "this release hasn't shipped yet".
CHANGELOG.rst
Outdated
| Major Features | ||
| -------------- | ||
|
|
||
| * Add Keyrings |
There was a problem hiding this comment.
Might be worth noting what Keyrings/ Keyring changes
CHANGELOG.rst
Outdated
| For backwards compatibility, | ||
| :class:`CryptoResult` also unpacks like a 2-member tuple. | ||
| This allows for backwards compatibility with the previous outputs | ||
| so this change should not break any existing consumers. |
| from aws_encryption_sdk.identifiers import Algorithm, ContentType, KeyringTraceFlag, ObjectType, SerializationVersion | ||
| from aws_encryption_sdk.structures import ( |
There was a problem hiding this comment.
| self.mock_stream_encryptor_instance.read.return_value = sentinel.ciphertext | ||
| self.mock_stream_encryptor_instance.header = sentinel.header |
There was a problem hiding this comment.
I assume these values used to be the same before?
There was a problem hiding this comment.
This was the "over-mocked" test I mentioned in the description. TLDR is that CryptoResult enforces input types and mock sentinels are not those types.
There was a problem hiding this comment.
Or, if you were asking if these values were similarly consistent, yes; mock sentinels are "an arbitrary named objects that are always the same within an execution".
Issue #, if available:
resolves: #223
Description of changes:
After discussing #223 with @WesleyRosenblum and @acioc, we agreed that going with option 2 made the most sense and did not introduce notable risk of breaking customers. The fact that none of the tests (aside from an over-mocked unit test) needed to be changed after I changed the return values supports this conclusion.
There are two main changes introduced here, both with the aim of making the keyring trace accessible to callers:
keyring_traceattribute as soon as they have the encryption/decryption materials.CryptoResultrather than a tuple.CryptoResultgives us the flexibility to add more output values in the future while also allowing unpacking and positional referencing just like the previous output (a 2-member tuple) did.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Check any applicable: